-
-
Notifications
You must be signed in to change notification settings - Fork 227
Relation controller popup: fix size, add cssClass and allowDismiss #1044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
@LukeTowers Should be finished |
|
Could add allowDismiss (see comments) |
|
@AIC-BV just a thought, is there a way we could integrate with the change monitor used in the cms section for the tabs to detect when a change has happened inside of a modal and warn / prevent the dismiss behaviour from being triggered in those cases? |
I assume something like this could be a basic for that kind of behaviour if (this.options.allowDismiss) {
modal.on('click', function(e) {
const target = e.target;
if (target.classList.contains('control-popup')) {
if (!modal.dataset.hasChanges) {
modal.hide()
$('.popup-backdrop').remove()
$(document.body).removeClass('modal-open')
} else {
// popup warning them about unsaved changes
}
}
});
const inputs = modal.querySelectorAll('input')
for (const input of inputs) {
input.addEventListener('change', function() {
modal.dataset.hasChanges = true
})
}
} |
|
Thoughts @LukeTowers @mjauvin ? |
|
@AIC-BV seems fine, have you tested it out? |
|
I'm using everything except the latest comment: #1044 (comment) |
|
@AIC-BV can we remove allowDismiss from this PR for now then? I don't want to support it unless it can be a bit safer first. |
|
If its ok for you I will add the popup shortly instead |
Can you help me getting started? Also, will this be enough? PS: |
|
There's existing code for tracking changes here: https://wintercms.com/docs/v1.2/ui/script/input-monitor |
|
Was making another relation popup and added these properties in the relation_config: I tried the change monitor and it doesn't show the warning when closing the popup.
What is left for finishing this PR is a way to show a popup warning the user about unsaved changes using either Input Monitor or custom written JS. |
|
@jaxwilko any input? |
You can use Apart from that looks cool, I'll check it out and have a play when I have time :) |
Only trigger on mousedown, because when selecting text to copy you sometimes leave the modal and that would close it.
|
Strongly suggest to try it out :) |
|
@AIC-BV are you able to record a loom or something showing the new functionality and make a PR to the docs? If you can get those done then I should be able to quickly review and merge this before I push out 1.2.7. |
allowDismiss = Clicking next to the modal closes it (on mousedown because if you drag to select something and release the mouse button outside the modal, it would close the modal too) |
|
Ran composer update and this got overwritten ofcourse. |
|
@AIC-BV waiting on the docs PR to get updated / my comment to be responded to: https://github.com/wintercms/docs/pull/207/files#r1739737060. Will also need to update from develop and recompile the JS. |
Oops 👀 |
|
@AIC-BV are we good with all the requested changes? I'd like this to get merged. |
I think there is this pending feedback but I don't know what to do with it wintercms/docs#207 |
@LukeTowers could you clarify so that can be completed? |
WalkthroughThe relation controller system is extended to support customizable popup modal styling and dismiss behavior. New parameters (size, cssClass, allowDismiss) are propagated from the PHP backend through the JavaScript relation handler to the Popup component, enabling configuration of modal appearance and backdrop-click dismissal directly from the RelationController. Changes
Sequence DiagramsequenceDiagram
participant User
participant RelationController as RelationController.php
participant ClickHandler as clickViewListRecord()
participant Popup as Popup Component
participant Modal as Modal DOM
User->>RelationController: Click list record
RelationController->>ClickHandler: Generate JS call with<br/>size, cssClass, allowDismiss
ClickHandler->>Popup: Create popup with new<br/>config parameters
Popup->>Modal: Apply cssClass to<br/>modal-dialog element
alt allowDismiss enabled
Popup->>Modal: Attach mousedown listener<br/>to backdrop
User->>Modal: Click backdrop area
Modal->>Popup: Trigger dismiss
Popup->>Modal: Remove modal & cleanup
end
Popup-->>User: Show styled modal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @modules/system/assets/ui/js/popup.js:
- Around line 218-227: The allowDismiss mousedown handler bypasses the Popup
lifecycle by calling jQuery's modal.hide() and manually removing backdrop/body
classes; replace that handler so it calls the Popup instance's hide() method
(use this.hide() bound to the popup context or call popup.hide()), remove manual
$('.popup-backdrop').remove() and $(document.body).removeClass('modal-open')
calls so Bootstrap/Popup cleanup and hide events run, and ensure the hide flow
honors the allowHide/lock checks (use this.lock(true) or check current lock
state before dismissing). If you prefer the safer option requested in PR, remove
or disable the allowDismiss feature entirely in this patch instead of
re-enabling it until InputMonitor (or equivalent unsaved-changes detection) is
integrated.
In @modules/system/assets/ui/storm-min.js:
- Around line 2102-2104: Remove the temporary allowDismiss implementation from
this PR by deleting the conditional block that attaches the mousedown handler
(the code using this.options.allowDismiss and modal.on('mousedown', ...)), and
remove any direct DOM manipulations inside it (the $('.popup-backdrop').remove()
and $(document.body).removeClass('modal-open') calls); leave Popup's existing
hide/dispose behavior untouched (do not add new event listeners or direct
backdrop/body manipulation here). If any configuration/defaults or docs were
added for allowDismiss, remove those as well. If reintroducing the feature
later, attach the handler to an instance property, check popup lock/state and
unsaved-change detection before calling modal.hide(), and remove the handler in
Popup.prototype.dispose rather than manipulating backdrop/body directly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
modules/backend/behaviors/RelationController.phpmodules/backend/behaviors/relationcontroller/assets/js/winter.relation.jsmodules/system/assets/ui/js/popup.jsmodules/system/assets/ui/storm-min.js
🧰 Additional context used
🧬 Code graph analysis (3)
modules/backend/behaviors/relationcontroller/assets/js/winter.relation.js (1)
modules/backend/classes/FormField.php (1)
size(217-221)
modules/system/assets/ui/js/popup.js (1)
modules/system/assets/ui/storm-min.js (1)
target(2102-2102)
modules/backend/behaviors/RelationController.php (1)
modules/backend/classes/FormField.php (1)
size(217-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (9)
modules/backend/behaviors/RelationController.php (2)
688-690: LGTM: Config properties correctly retrieved.The three new configuration properties (size, cssClass, allowDismiss) are properly retrieved from the manage section of the relation config. These values default to null if not specified, which is handled appropriately by the downstream JavaScript code.
692-700: LGTM: Parameter passing is consistent.The sprintf format string and arguments are correctly aligned. The six parameters match the updated JavaScript function signature in winter.relation.js, enabling the popup configuration to flow from the backend to the frontend.
modules/system/assets/ui/js/popup.js (2)
52-54: LGTM: Safe default values.The new default values are appropriately conservative. Setting allowDismiss to false by default ensures existing popups maintain their current behavior and only opt-in to the new dismiss functionality.
212-213: LGTM: cssClass implementation is safe.The cssClass is correctly applied to the modal-dialog element. jQuery's addClass method safely handles the input without XSS risk.
modules/backend/behaviors/relationcontroller/assets/js/winter.relation.js (2)
12-12: LGTM: Function signature extended correctly.The updated signature accepts the three new parameters (size, cssClass, allowDismiss) that are now passed from the PHP backend. This maintains consistency across the backend-to-frontend data flow.
17-25: The code correctly uses 'huge' as a valid popup size with appropriate CSS styling.The default size of 'huge' is valid and well-established in the codebase, with CSS class definitions present in multiple stylesheet files. The popup configuration correctly passes all three parameters (size, cssClass, allowDismiss) to the popup plugin.
modules/system/assets/ui/storm-min.js (3)
2100-2100: LGTM: cssClass implementation looks correct.The cssClass option is properly applied to the modal dialog element, allowing custom styling as documented in the PR objectives.
4683-4683: Clarify unrelated change to listSortable.This change to the listSortable component appears unrelated to the PR's objectives (relation controller popup configuration). Is this an unintentional side effect from recompiling the minified file, or does it serve a purpose for this PR?
If unintentional, consider reverting this change to keep the PR focused on relation popup enhancements.
Could you confirm whether this change was intentional and how it relates to the popup size/cssClass/allowDismiss features?
2056-2056: Consider disabling or enhancing allowDismiss with change detection.The
allowDismissimplementation allows users to dismiss popups by clicking the backdrop without validating for unsaved changes. While the feature defaults tofalse(opt-in), if enabled it should integrate with the Input Monitor system to prevent accidental data loss.The
cssClassaddition is good and can remain.Suggested improvements if allowDismiss is kept:
- Integrate with the Input Monitor (input.monitor.js) to detect unsaved changes before allowing dismissal
- Show a confirmation dialog if changes are detected
- Clean up the
mousedownevent listener on popup disposal to prevent memory leaks
| if (this.options.allowDismiss) { | ||
| modal.on('mousedown', function(e) { | ||
| const target = e.target; | ||
| if (target.classList.contains('control-popup')) { | ||
| modal.hide() | ||
| $('.popup-backdrop').remove() | ||
| $(document.body).removeClass('modal-open') | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: allowDismiss implementation bypasses popup lifecycle and lock mechanism.
The current implementation has several critical flaws:
-
Line 222 uses
modal.hide()(jQuery's hide method) instead of calling the Popup instance'shide()method (line 334). This bypasses:- The
allowHidelock check (line 340) which prevents closing locked popups - The hide event triggers (lines 337-338)
- The proper cleanup flow managed by Bootstrap modal events
- The
-
Lines 223-224 manually remove backdrop and body class instead of letting the Bootstrap modal hide events handle cleanup (lines 148-154). This can leave the popup in an inconsistent state.
-
Missing unsaved changes protection: Per PR discussion, there is no change monitoring to warn users about unsaved data before dismissing. This creates a risk of accidental data loss.
Recommendation: Based on the PR discussion where LukeTowers requested removing allowDismiss until safer handling is implemented, consider either:
- Remove the allowDismiss feature from this PR
- Integrate Winter UI Input Monitor to detect unsaved changes before allowing dismiss
- Use the popup's lock mechanism (
this.lock(true)) when changes are detected
🔧 Proposed fix to use proper popup hide method
if (this.options.allowDismiss) {
+ var self = this
modal.on('mousedown', function(e) {
const target = e.target;
if (target.classList.contains('control-popup')) {
- modal.hide()
- $('.popup-backdrop').remove()
- $(document.body).removeClass('modal-open')
+ // Use the popup instance's hide method to ensure proper cleanup
+ self.hide()
}
});
}Note: This fix addresses the lifecycle bypass but does not solve the missing change monitoring concern.
🤖 Prompt for AI Agents
In @modules/system/assets/ui/js/popup.js around lines 218 - 227, The
allowDismiss mousedown handler bypasses the Popup lifecycle by calling jQuery's
modal.hide() and manually removing backdrop/body classes; replace that handler
so it calls the Popup instance's hide() method (use this.hide() bound to the
popup context or call popup.hide()), remove manual $('.popup-backdrop').remove()
and $(document.body).removeClass('modal-open') calls so Bootstrap/Popup cleanup
and hide events run, and ensure the hide flow honors the allowHide/lock checks
(use this.lock(true) or check current lock state before dismissing). If you
prefer the safer option requested in PR, remove or disable the allowDismiss
feature entirely in this patch instead of re-enabling it until InputMonitor (or
equivalent unsaved-changes detection) is integrated.
| if(this.options.allowDismiss){modal.on('mousedown',function(e){const target=e.target;if(target.classList.contains('control-popup')){modal.hide() | ||
| $('.popup-backdrop').remove() | ||
| $(document.body).removeClass('modal-open')}});}return modal.append(modalDialog.append(modalContent))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: allowDismiss implementation lacks change detection and proper cleanup.
Multiple concerns with the current allowDismiss implementation:
- Data loss risk: No validation for unsaved changes before closing (the primary concern raised in PR discussion)
- Memory leak: The mousedown event listener is never removed when the popup is disposed
- Direct DOM manipulation: Directly manipulating backdrop and body classes may conflict with the Popup's internal state management
The event listener should be stored and removed in the dispose method. However, per maintainer feedback, the entire allowDismiss feature should be removed from this PR until proper change-monitoring integration is complete.
🐛 Issues to address if keeping this feature
If allowDismiss is kept (against maintainer's request), at minimum:
- Store the event handler reference and remove it in
Popup.prototype.dispose - Check if the popup is locked before allowing dismissal
- Integrate change detection to prevent data loss
- Use the Popup's own hide method logic rather than direct DOM manipulation
However, the recommended action is to remove this feature entirely until proper change-monitoring is implemented.
Based on PR discussion where LukeTowers requested removal of allowDismiss until safer implementation.
🤖 Prompt for AI Agents
In @modules/system/assets/ui/storm-min.js around lines 2102 - 2104, Remove the
temporary allowDismiss implementation from this PR by deleting the conditional
block that attaches the mousedown handler (the code using
this.options.allowDismiss and modal.on('mousedown', ...)), and remove any direct
DOM manipulations inside it (the $('.popup-backdrop').remove() and
$(document.body).removeClass('modal-open') calls); leave Popup's existing
hide/dispose behavior untouched (do not add new event listeners or direct
backdrop/body manipulation here). If any configuration/defaults or docs were
added for allowDismiss, remove those as well. If reintroducing the feature
later, attach the handler to an instance property, check popup lock/state and
unsaved-change detection before calling modal.hide(), and remove the handler in
Popup.prototype.dispose rather than manipulating backdrop/body directly.
Add the possibility to define the popup size and its custom CSS classes.
You can now define these properties at the manage part of the config_relation.yaml:
See all popup sizes at https://wintercms.com/docs/v1.2/ui/controls/popup#data-attributes
sizealready existed, but was hardcoded and not dynamically filled in.cssClassis added.Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.